First attempt at providing support for conditional dependencies #1078
Conversation
|
|
||
|
|
||
| def flat_deps(services, with_extends=False): | ||
| """ | ||
| create dependencies "_deps" or update it recursively for all services | ||
| """ | ||
| def rec_deps(services, service_name, start_point=None): |
There was a problem hiding this comment.
Moving this function made reviewing way harder.
There was a problem hiding this comment.
Noted, but... do you want me to move it out? Because it makes full sense to have it there...
There was a problem hiding this comment.
I would say it makes things more confusing. When functions are moved to other functions it's often because they need to refer to variables of parent function. The dependencies are less clear.
There was a problem hiding this comment.
Ok: I will move it out, because it's not the point to have a discussion about this :-) You are right when you say that "it's often because they need to refer to variables of parent function" (although this is not the case here), but inner functions are also used when you want to encapsulate functionality on its own function but still not make it public to be called from anywhere else.
| @@ -1048,8 +1049,8 @@ async def container_to_args(compose, cnt, detached=True): | |||
| if pod: | |||
| podman_args.append(f"--pod={pod}") | |||
| deps = [] | |||
| for dep_srv in cnt.get("_deps", None) or []: | |||
There was a problem hiding this comment.
I would move this specific refactoring into a separate commit for easier review.
| for dep_srv in cnt.get("_deps", None) or []: | ||
| deps.extend(compose.container_names_by_service.get(dep_srv, None) or []) | ||
| for dep_srv in cnt.get("_deps", []): | ||
| deps.extend(compose.container_names_by_service.get(dep_srv.name, [])) |
There was a problem hiding this comment.
I would move this one into separate commit as well. If the PR breaks something, then it's possible to bisect in smaller increments.
There was a problem hiding this comment.
But I am lost now, with respect to this (and the previous) comment: if this PR breaks something, it should be entirely reverted, and done. I can agree to the statement "these refactorings should not be here, but in a separate PR", and I will remove them (and, honestly, most likely not bring them back anymore... because this is just a cosmetic change aimed at simplifying the code, but I will not spend the time to go through a full review process for changes like this... I hope you do not take me wrong).
But, besides this: I'd expect to have all the commits squashed into one, meaning that bisection of this added feature is not possible anyway (as there is a single commit, and is a fairly small feature). What is the policy on this project for the contributions? Rebasing the feature branch into master? This potentially prevents developers from committing often.
There was a problem hiding this comment.
if this PR breaks something, it should be entirely reverted, and done
I agree, but often it's not possible. For example, if the feature is already released, then people may start depending on it. Thus some edge case breaking results in maintainer being caught between a rock and a hard plate. The code can't be left as is, but it can't be reverted either. So I will need to dig and do fixes.
"these refactorings should not be here, but in a separate PR",
Sorry for being unclear, I didn't mean this. I just wanted a separate commit(s) in the PR. The sequence of commits would then be merged as is, without squashing. I agree that creating many separate PRs is good use of time.
but I will not spend the time to go through a full review process for changes like this... I hope you do not take me wrong).
Fair. It's perhaps because I'm myself so accustomed to rebase-based workflow that massaging commit history does not take a lot of time for me. So if I see that I am going to need certain refactoring, I firstly do that refactoring, create a commit and then continue working on where I was left.
What is the policy on this project for the contributions? Rebasing the feature branch into master?
The policy is having PRs that consist of a sequence of commits, each of which do single thing. The PR is then merged as is, with a merge commit. If there needs any additional rebasing usually I do it myself so that contributors don't need to bother.
There was a problem hiding this comment.
Noted. Considering the different flavours of coding / contributing out there, maybe is a good idea to add this to the CONTRIBUTING.md file?
Signed-off-by: Felix Rubio <[email protected]>
Signed-off-by: Felix Rubio <[email protected]>
Signed-off-by: Felix Rubio <[email protected]>
Signed-off-by: Felix Rubio <[email protected]>
Signed-off-by: Felix Rubio <[email protected]>
Signed-off-by: Felix Rubio <[email protected]>
Signed-off-by: Felix Rubio <[email protected]>
Signed-off-by: Felix Rubio <[email protected]>
|
@p12tic I am closing the PR to put it back when all is working with the changes you requested. |
Currently all services dependencies are treated unconditionally. However, docker-compose supports specifying three possible conditions (started, healthy, successfully finished). This PR attempts to provide such support, extending the existing unit tests for dependencies.